Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Throw errors during Tabs initialisation if key HTML elements are missing #4263

Merged
merged 6 commits into from
Sep 27, 2023
Merged

Throw errors during Tabs initialisation if key HTML elements are missing #4263

merged 6 commits into from
Sep 27, 2023

Conversation

domoscargin
Copy link
Contributor

@domoscargin domoscargin commented Sep 26, 2023 •
edited
Loading

Closes #4127

A coupla notes:

  • So far, I've limited this PR to replacing empty returns in the initialisation of the component. There are 17 more empty returns in the component, some of which are bound during initialisation. These are in event handlers, however, which we can refactor as and when we add things to the API.
  • I've removed a check in setup() as unnecessary. Some checks will need to replace it if setup() becomes an API method that can be called separately from initialisation, but I figure we'd probably do a slight code refactor then anyway.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4263 September 26, 2023 09:59 Inactive
@github-actions
Copy link

github-actions bot commented Sep 26, 2023 •
edited
Loading

📋 Stats

File sizes

File Size
dist/govuk-frontend-4.6.0.min.css 118.07 KiB
dist/govuk-frontend-4.6.0.min.js 47.1 KiB
dist/govuk-frontend-ie8-4.6.0.min.css 79.27 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 77.65 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 72.94 KiB
packages/govuk-frontend/dist/govuk/all.mjs 3.84 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 120.45 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 37.89 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.83 KiB

Modules

File Size
all.mjs 69.04 KiB
components/accordion/accordion.mjs 21.91 KiB
components/button/button.mjs 4.71 KiB
components/character-count/character-count.mjs 22.03 KiB
components/checkboxes/checkboxes.mjs 5.69 KiB
components/error-summary/error-summary.mjs 6.01 KiB
components/exit-this-page/exit-this-page.mjs 16.61 KiB
components/header/header.mjs 3.84 KiB
components/notification-banner/notification-banner.mjs 4.55 KiB
components/radios/radios.mjs 4.68 KiB
components/skip-link/skip-link.mjs 3.74 KiB
components/tabs/tabs.mjs 9.32 KiB

View stats and visualisations on the review app

Action run for 1377822

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4263 September 26, 2023 13:07 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4263 September 26, 2023 13:08 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4263 September 26, 2023 13:14 Inactive
@colinrotherham colinrotherham mentioned this pull request Sep 26, 2023
Merged
colinrotherham
colinrotherham reviewed Sep 26, 2023
View reviewed changes
packages/govuk-frontend/src/govuk/components/tabs/tabs.mjs
'.govuk-tabs__list-item'
)

if (!this.$tabs || !$tabList || !$tabListItems) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

@domoscargin domoscargin marked this pull request as ready for review September 26, 2023 14:45
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4263 September 26, 2023 14:45 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4263 September 26, 2023 14:58 Inactive
colinrotherham
colinrotherham reviewed Sep 27, 2023
View reviewed changes
Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking ace @domoscargin

Just merged #4261 so let's give this one a rebase and drop the [data-module="${Tabs.moduleName}"] prefix then think we're good to go 🚀

@domoscargin
Throw error when tabs list is empty
2f8af75
@domoscargin
Throw errors for missing tab list and items
b7e50d3
@domoscargin
Remove active tab check
3fe19cb 
Since `setup()` is run during initialisation, we've already validated `this.$tabs` is not empty, and thrown if it is.

As a result, `$activeTab` will always be set to SOMETHING, since we know `this.$tabs[0]` is valid.

If `setup()` were to become an API method that could be called separately, we'd need to validate the `$tabs` list in some way anyway.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4263 September 27, 2023 08:12 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4263 September 27, 2023 08:18 Inactive
colinrotherham
colinrotherham approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved

(One little comment, but only a quick tweak)

packages/govuk-frontend/src/govuk/components/tabs/tabs.mjs Outdated Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4263 September 27, 2023 08:36 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4263 September 27, 2023 08:38 Inactive
@domoscargin domoscargin merged commit ca02f0d into main Sep 27, 2023
44 checks passed
@domoscargin domoscargin deleted the bk-tabs-errors branch September 27, 2023 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romaricpascal romaricpascal romaricpascal left review comments

@colinrotherham colinrotherham colinrotherham approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Throw errors during Tabs initialisation if key HTML elements are missing
4 participants
@domoscargin @romaricpascal @colinrotherham @govuk-design-system-ci